Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Production Mode via DEBUG or docker-compose.ci.yml #22613

Merged
merged 11 commits into from
Sep 5, 2024

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Aug 30, 2024

Relates to: mozilla/addons#14972

Description

This PR achieves 2 goals.

  1. Makes running addons-server in DEBUG=false i.e. "production" mode possible
  2. Fixes existing issues with serving static files in development mode

The specific changes include:

  • respecting host defined DEBUG variable to run the container in. when debug true enable serving static files from ./static directory mounted from host as well as from django static server to load dependency assets.
  • modifying the way we mount static files to ensure that development mode does not require subsequent calls to update_assets in order to run addons-server and that production mode contains necessary and collected static files from site-static.
  • using a package django-node-assets to load node_modules as static assets directly from our node_modules directory. This simplifies our setup code, and like the previous step ensures we do not need additional commands to run the containers in dev mode.
  • move assets that we "build" ourselves to a separate directory, that is added to STATICFILES_DIRS. The allows us to freely mount ./static in development without overriding built assets.

Context

Why not just mount ./static and be done with it? A few problems

  • during our docker build we copy ./static but then we modify and add files. If you mount ./static again after building, the host files (which do not have the "build" assets) get removed.. so you end up back at the beginning.
  • some files are available in ./static and some are in django dependencies or npm dependencies. We were using some complex scripting and mounting to make them available, but this can be simplified for development where we don't need collected static assets.

Why not just use runserver or serve for all assest

We use uwsgi to run django even in development because this more closely mirrors how our app runs in prod. We also cannot always rely on static_serve because in DEBUG=False (production mode) this will either not work, or even if we force it, it will be missing files.

The simpler solution is to

  1. load files from whatever is in nginx locally
  2. fallback to a request to addons-server static serve.

In production mode, 1 will have all the files and so it's fine that 2 will always fail.
In development mode, 1 will have most of the files from ./static which is fast and any files that are missing will be served from serve_static.

Testing

You should run several different iterations of addons-server and verify each is able to serve the expected static assets. This can be verified by running:

  • login flow
  • devhub (just load pages, it should all work)
  • admin (/admin page should load)
  • swagger (/api/v5/swagger. page should load)

Each scenario is toggling whether we target development or production dependencies, and whether to run in DEBUG mode or not. These are independent features and can be toggled accordingly.

Additionally you can set DEBUG directly in make commands by adding it as an argument, but the value is by default set to true and when docker-compose.ci.yml is included in the project the value is hard coded to "DEBUG=" regardless of the value set in the host environment or arguments.

IMPORTANT: before running each test you should "make down" to clear any image/volume artifacts, make sure to use exact versions since you are changing a lot between each run. After running make up, you should run "make reindex" to repopulate elasticsearch indexes as they are cleared when you make down and currently not repopulated automatically

  1. Development/DEBUG=True
make up DOCKER_VERSION=local DOCKER_TARGET=development COMPOSE_FILE=docker-compose.yml
  1. Development/DEBUG=
make up DOCKER_VERSION=local DOCKER_TARGET=development COMPOSE_FILE=docker-compose.yml:docker-compose.ci.yml
  1. Production/DEBUG=True
make up DOCKER_VERSION=local DOCKER_TARGET=production COMPOSE_FILE=docker-compose.yml
  1. Production/DEBUG=
make up DOCKER_VERSION=local DOCKER_TARGET=production COMPOSE_FILE=docker-compose.yml:docker-compose.ci.yml

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind requested review from a team and diox and removed request for a team August 30, 2024 10:56
Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned on slack, we need a way for assets to load using DEBUG=False. SERVE_STATIC variable is a good start but not enough: At the moment, with this patch, if I've got DEBUG=False set, then the URLs we are going to be referencing are going to be the ones with the hash that are in site-static/, but serve() only knows how to serve the source stuff from static/, not the built ones.

With DEBUG=False and SERVE_STATIC=True http://olympia.test/static/img/favicon.3cec07462a3d.ico fails for me for instance (and all js/css/images as well obviously).

We need some way of handling that scenario (where the developer wants a configuration closer to production - with "real" compiled assets, while still being able to develop locally, either to build some feature/fix some bug that requires real assets, or to reproduce an issue with production).

@KevinMind KevinMind changed the title Serve static file directly from django in development Enable Production Mode via DEBUG or docker-compose.ci.yml Sep 3, 2024
@KevinMind KevinMind force-pushed the serve-static branch 3 times, most recently from e08aef7 to b121b2a Compare September 3, 2024 11:12
@KevinMind KevinMind marked this pull request as draft September 3, 2024 12:29
@KevinMind KevinMind requested a review from diox September 3, 2024 13:23
@KevinMind KevinMind marked this pull request as ready for review September 3, 2024 13:23
@KevinMind KevinMind force-pushed the serve-static branch 6 times, most recently from 060bbee to 9303ba5 Compare September 4, 2024 16:53
@KevinMind
Copy link
Contributor Author

@diox it is ready for review and QA. Note, you either need a stage account for fxa or you need to force use_fake_fxa to return true for the 2nd and 4th case where we use docker-compose.ci.yml as this sets DEBUG= and will trigger real auth.

@KevinMind KevinMind force-pushed the serve-static branch 3 times, most recently from 3995433 to c6b2997 Compare September 5, 2024 10:08
src/olympia/lib/settings_base.py Outdated Show resolved Hide resolved
src/olympia/urls.py Show resolved Hide resolved
docker/nginx/addons.conf Show resolved Hide resolved
@@ -87,8 +92,7 @@ services:
image: nginx
volumes:
- ./docker/nginx/addons.conf:/etc/nginx/conf.d/addons.conf
- ./static:/srv/static
- ./site-static:/srv/site-static
- ./static:/srv/site-static
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing me - Why not keep /srv/static (and keeping the old alias directive intact in addons.conf) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we mount either "./static" or "/data/olympia/site-static" depending on which set of compose files you use.

As far as nginx is concerned it serves from one location but the source of the files changes.

docs/topics/development/static-files.md Outdated Show resolved Hide resolved
docs/topics/development/static-files.md Show resolved Hide resolved
@KevinMind KevinMind merged commit 2a64618 into master Sep 5, 2024
31 checks passed
@KevinMind KevinMind deleted the serve-static branch September 5, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants